[RemoveLayoutConversions]: Reduce loop carried values - part 2 #4921
Conversation
Signed-off-by: Tiotto, Ettore <[email protected]>
Signed-off-by: Tiotto, Ettore <[email protected]>
Signed-off-by: Tiotto, Ettore <[email protected]>
Signed-off-by: Tiotto, Ettore <[email protected]>
|
Depends on PR #4915. |
There was a problem hiding this comment.
Pull Request Overview
This PR implements functionality to reduce loop carried values in the RemoveLayoutConversions pass by eliminating unnecessary loop-carried tensor pointer values when they can be reconstructed from other values plus layout conversions.
- Adds a new
reduceLoopCarriedValues()method that identifies loop arguments that can be eliminated - Implements logic to replace operations using eliminated values with equivalent operations using rematerialized values plus convert layout operations
- Updates the backward rematerialization process to include the loop carried value reduction optimization
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| RemoveLayoutConversions.cpp | Implements the core logic for reducing loop carried values by adding the new method and integrating it into the backward rematerialization process |
| backward_combine_dpas_dot_layout.mlir | Adds test cases to verify the optimization correctly reduces loop carried values and generates expected layout conversions |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| // Replace the loop result corresponding to the argument with an | ||
| // equivalent loop result. | ||
| OpResult rematRes = loopOp.getTiedLoopResult(cast<BlockArgument>(val)); |
There was a problem hiding this comment.
The cast(val) is redundant and potentially unsafe. The variable val is already confirmed to be of type Value from the rematMapping, but the code already has arg which is the properly cast BlockArgument from line 1020. Use arg instead of casting val.
| OpResult rematRes = loopOp.getTiedLoopResult(cast<BlockArgument>(val)); | |
| OpResult rematRes = loopOp.getTiedLoopResult(arg); |
Copilot uses AI. Check for mistakes.
| .Default([](auto op) { | ||
| llvm::report_fatal_error(llvm::Twine( | ||
| "Unsupported operation in backward rematerialization: '" + | ||
| op->getName().getStringRef() + "'")); |
There was a problem hiding this comment.
The error message in the Default case uses string concatenation with Twine which may not work as expected. Consider using llvm::formatv or constructing the error message differently: llvm::formatv("Unsupported operation in backward rematerialization: '{0}'", op->getName().getStringRef())
| op->getName().getStringRef() + "'")); | |
| llvm::report_fatal_error( | |
| llvm::formatv("Unsupported operation in backward rematerialization: '{0}'", op->getName().getStringRef())); |
Copilot uses AI. Check for mistakes.
TBD